feat: move unity catalog integration to it's own crate #3044

hntd187 · 2024-12-02T15:23:32Z

Description

This moves the catalog integration of unity catalog to it's own crate. It also replaces the mock server with the httpmock crate and the custom retry logic with request_middleware's retry middleware. I think this will be easier to manage as a client and I do not believe many people were using this integration anyways.

Related Issue(s)

Documentation

codecov · 2024-12-02T15:37:15Z

Codecov Report

Attention: Patch coverage is 53.72340% with 87 lines in your changes missing coverage. Please review.

Project coverage is 72.40%. Comparing base (a51d75a) to head (93701f4).

Files with missing lines	Patch %	Lines
crates/catalog-unity/src/client/retry.rs	14.89%	40 Missing ⚠️
crates/catalog-unity/src/credential.rs	70.90%	7 Missing and 9 partials ⚠️
crates/catalog-unity/src/lib.rs	67.39%	11 Missing and 4 partials ⚠️
crates/catalog-unity/src/client/mod.rs	64.70%	4 Missing and 2 partials ⚠️
crates/catalog-unity/src/datafusion.rs	0.00%	3 Missing ⚠️
crates/core/src/delta_datafusion/expr.rs	0.00%	3 Missing ⚠️
crates/core/src/kernel/models/actions.rs	50.00%	3 Missing ⚠️
crates/core/src/operations/merge/filter.rs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3044      +/-   ##
==========================================
- Coverage   72.65%   72.40%   -0.25%     
==========================================
  Files         129      128       -1     
  Lines       41584    41314     -270     
  Branches    41584    41314     -270     
==========================================
- Hits        30211    29915     -296     
- Misses       9405     9430      +25     
- Partials     1968     1969       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crates/catalog-unity/src/client/retry.rs

crates/catalog-unity/src/credential.rs

rtyler · 2024-12-06T03:16:45Z

crates/catalog-unity/src/credential.rs

-                r#"
+        let client = reqwest_middleware::ClientBuilder::new(Client::new()).build();
+
+        let _retry_config = RetryConfig::default();


What's this for?

I wasn't sure if I was going to remove it at that point from the testing, so it's a side effect left over from that.

rtyler · 2024-12-06T03:17:00Z

crates/catalog-unity/src/credential.rs

-            Response::new(Body::from(
-                r#"
+        let client = reqwest_middleware::ClientBuilder::new(Client::new()).build();
+        let _retry_config = RetryConfig::default();


Another unused variable that should be removed? 🤔

crates/core/Cargo.toml

rtyler · 2024-12-06T03:19:45Z

crates/core/Cargo.toml

@@ -137,4 +138,4 @@ datafusion = [
 datafusion-ext = ["datafusion"]
 json = ["parquet/json"]
 python = ["arrow/pyarrow"]
-unity-experimental = ["reqwest", "hyper"]
+unity-experimental = ["reqwest", "reqwest-middleware"]


Shouldn't this feature come off the core crate now that this code is moved into its own crate?

So this is a discussion point I wanted to talk about. The main trait for the catalog and the errors remained in the core crate. So core can build I assume generic catalog support and catalog crates just depend on core and implement. At first glance I kinda didn't have a cohesive error handling thing here, but I think thinking about it. There should be a generic catalog error in core, but have no conditionals features on it. And individual crates implement a conversion to that error type. Sound good?

ion-elgreco · 2024-12-10T21:08:45Z

crates/core/src/operations/merge/filter.rs

-            let compare_expr = match generalize_filter(
+            let compare_expr = generalize_filter(


Why is this changed?

I fixed a few warnings along the way, this statement could be just written with an ? operator instead.

roeap

Overall looks good, just some minor questions.

More gene4rally speaking though, I think we need to make some more deliberate choices around how we want to integrate with catalogs.

AS I have been out for a while, I am not sure where we are in terms of eagerness to push the next release. I would hope that we can do some more cleaning and most importantly testing before we release get "more serious" with unity ...

w.r.t. testing I hope that we can even do some integration tests using OSS UC. Happy the help with that.

roeap · 2024-12-10T20:58:15Z

crates/catalog-unity/src/lib.rs

@@ -216,7 +226,10 @@ impl FromStr for UnityCatalogConfigKey {
            "workspace_url" | "unity_workspace_url" | "databricks_workspace_url" => {
                Ok(UnityCatalogConfigKey::WorkspaceUrl)
            }
-            _ => Err(UnityCatalogError::UnknownConfigKey(s.into()).into()),
+            _ => Err(DataCatalogError::UnknownConfigKey {
+                catalog: "",


I guess in this case the "catalog" would be "Unity".

Btw... a lot fi the client impl was just stolen from Object store, where this field identifies the specific implementation (azure, gcp, ...). If we have this error live in a sepate create, this dioes not make much sense anymore.

roeap · 2024-12-10T21:00:13Z

crates/catalog-unity/src/lib.rs


-        Ok(resp.json().await?)
+        Ok(resp.json().await.map_err(UnityCatalogError::from)?)


if we have an impl From<...> for the error, why do we need the map? does the function return a different kind of err.

This is a bit of jank, the base error type in core no longer has a from for rewqest::Error so we convert to our own error type that does have a From to DataCatalogError

roeap · 2024-12-10T21:09:32Z

crates/gcp/tests/context.rs

-            "GOOGLE_SERVICE_ACCOUNT",
-            account_path.as_path().to_str().unwrap(),
-        );
+        unsafe {


Am I missing something, or did we remove unsafe around set_var further above, specifically in crates/aws/src/storage.rs, and here we are adding it?

I have to add it back to the other one, thanks for pointing it out

roeap

LGTM!

One ore thing though, and not sure if this would actually happen, but it seems GH wants to re-apply a bunch of commits already on main. If all of these were duplicated, this would give us (IMO) a fairly dirty history. Could we squash these to make it a bit cleaner?

hntd187 · 2024-12-14T07:22:36Z

@roeap yes, I'll squash prior to merge

rtyler · 2024-12-16T15:03:14Z

@hntd187 unfortunately it looks like this needs to be rebased again 🤦

Signed-off-by: Stephen Carman <[email protected]>

rtyler · 2024-12-16T15:06:18Z

I have resolved conflicts, I will push and land this before I cut another release today

github-actions bot added binding/python Issues for the Python package binding/rust Issues for the Rust crate labels Dec 2, 2024

omkar-foss mentioned this pull request Dec 3, 2024

Add Python-side support for Unity Catalog #3046

Open

rtyler reviewed Dec 6, 2024

View reviewed changes

hntd187 marked this pull request as ready for review December 10, 2024 20:44

hntd187 requested review from wjones127, fvaleye, roeap and ion-elgreco as code owners December 10, 2024 20:44

ion-elgreco reviewed Dec 10, 2024

View reviewed changes

roeap reviewed Dec 10, 2024

View reviewed changes

hntd187 force-pushed the move-unity branch from 514c286 to aba99ed Compare December 13, 2024 19:15

roeap previously approved these changes Dec 13, 2024

View reviewed changes

hntd187 dismissed roeap’s stale review via cfc6b30 December 15, 2024 17:21

hntd187 force-pushed the move-unity branch from 52b3cdc to cfc6b30 Compare December 15, 2024 17:21

hntd187 enabled auto-merge December 15, 2024 22:51

hntd187 requested review from ion-elgreco, roeap and rtyler December 15, 2024 22:52

rtyler approved these changes Dec 16, 2024

View reviewed changes

hntd187 added this pull request to the merge queue Dec 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Dec 16, 2024

hntd187 and others added 4 commits December 16, 2024 15:04

feat: rust 2024 edition, moved unity to its own crate

7e74b38

Signed-off-by: Stephen Carman <[email protected]>

feat: move unity catalog integration into its own crate

a625768

Signed-off-by: Stephen Carman <[email protected]>

feat: move unity catalog integration into its own crate

92b45a0

Signed-off-by: Stephen Carman <[email protected]>

feat: move unity catalog integration into its own crate

a3f502b

Signed-off-by: Stephen Carman <[email protected]>

rtyler force-pushed the move-unity branch from 93701f4 to a3f502b Compare December 16, 2024 15:13

rtyler enabled auto-merge December 16, 2024 15:13

rtyler added this pull request to the merge queue Dec 16, 2024

Merged via the queue into delta-io:main with commit a5d0097 Dec 16, 2024
21 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move unity catalog integration to it's own crate #3044

feat: move unity catalog integration to it's own crate #3044

hntd187 commented Dec 2, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

rtyler Dec 6, 2024

hntd187 Dec 7, 2024

rtyler Dec 6, 2024

rtyler Dec 6, 2024

hntd187 Dec 7, 2024

ion-elgreco Dec 10, 2024

hntd187 Dec 11, 2024

roeap left a comment

roeap Dec 10, 2024

roeap Dec 10, 2024

hntd187 Dec 11, 2024

roeap Dec 10, 2024

hntd187 Dec 11, 2024

roeap left a comment •

edited

Loading

hntd187 commented Dec 14, 2024

rtyler commented Dec 16, 2024

rtyler commented Dec 16, 2024

		let compare_expr = match generalize_filter(
		let compare_expr = generalize_filter(


		Ok(resp.json().await?)
		Ok(resp.json().await.map_err(UnityCatalogError::from)?)

feat: move unity catalog integration to it's own crate #3044

feat: move unity catalog integration to it's own crate #3044

Conversation

hntd187 commented Dec 2, 2024

Description

Related Issue(s)

Documentation

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeap left a comment • edited Loading

Choose a reason for hiding this comment

hntd187 commented Dec 14, 2024

rtyler commented Dec 16, 2024

rtyler commented Dec 16, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

roeap left a comment •

edited

Loading